add index metrics #85

matsumana · 2017-08-16T12:34:41Z

I would like to monitor index metrics, just like Kinaba X-pack monitoring.
so I added 3 metrics below:

# HELP elasticsearch_indices_docs_primary Count of documents which only primary shards
# TYPE elasticsearch_indices_docs_primary gauge
elasticsearch_indices_docs_primary{cluster="test",index="foo_1"} 2
elasticsearch_indices_docs_primary{cluster="test",index="foo_2"} 3

# HELP elasticsearch_indices_store_size_bytes_primary Current total size of stored index data in bytes which only primary shards on all nodes
# TYPE elasticsearch_indices_store_size_bytes_primary gauge
elasticsearch_indices_store_size_bytes_primary{cluster="test",index="foo_1"} 8425
elasticsearch_indices_store_size_bytes_primary{cluster="test",index="foo_2"} 12420

# HELP elasticsearch_indices_store_size_bytes_total Current total size of stored index data in bytes which all shards on all nodes
# TYPE elasticsearch_indices_store_size_bytes_total gauge
elasticsearch_indices_store_size_bytes_total{cluster="test",index="foo_1"} 16850
elasticsearch_indices_store_size_bytes_total{cluster="test",index="foo_2"} 24840

dominikschulz

LGTM

matsumana · 2017-08-21T08:43:47Z

Hello, Please review.

metalmatze

We should either remote all indices metric from collector/nodes.go and move them into collector/indices.go or the other way around. But like this it gets confusing.

zwopir

the metrics your are using from _all/_stats are all included in _nodes/_local/stats/_nodes/stats which we already get from ES in `collector/nodes.go. Some of the json fields are not (yet) used, but if you are not planning to use the detailed index stats, we don't need to make another http call.

The additional call can be still useful, if we evaluate the per index metrics. I would then make the _all/_stats call optional.

So my suggestion is:

retrieve the metrics from this PR from the _nodes/stats endpoint, i.e. extend collector/nodes.go.
pick a (for now arbitrary) metrics from the index endpoint and expose it as prometheus metric as a starting point for future word. We can then later extend indices.go to expose detailed indices metrics.

After merging this PR I would implement the opt-in/opt-out for sub collectors.

…ces.go`

matsumana · 2017-08-23T13:02:16Z

Hello, Thank you for the comments.

the metrics your are using from _all/_stats are all included in _nodes/_local/stats/_nodes/stats which we already get from ES in `collector/nodes.go.

Certainly we can get the metrics of docs from _nodes/_local/stats / _nodes/stats.
however, which is the docs metric of primary + replica. I want to collect metrics of primary docs only.
Actually, it seems that Kibana X-Pack Monitoring is so.

matsumana · 2017-08-23T13:04:50Z

So I fixed below:

moved all indices metric from collector/nodes.go to collector/indices.go
gathered the metrics collect logic in metrics_collector. To avoid redundant api calls.

What do you think?

need to have a look at the changes

zwopir · 2017-08-25T11:25:22Z

not sure if moving the index metrics to a another call to the ES api is what we want. We need to call _nodes/_local/stats or depending on the all-flag _nodes/stats for the nodes-metrics anyways. I would like to make the indices-metrics optional, since people might have many indices and we thus create a high metrics cardinality and much more json to transfer and parse.

What do you think, @dominikschulz ?

dominikschulz · 2017-08-25T14:07:45Z

Sounds reasonable. We should make indices metrics optional.

matsumana · 2017-08-26T16:31:34Z

@zwopir @dominikschulz
I agree with you.
Actually, I have heard that there are Elasticsearch clusters with several hundreds indexes in some companies.
I fixed this PR. Please review.

dominikschulz · 2017-08-28T12:36:59Z

@matsumana IMHO you're changing/moving a lot of unrelated code that shouldn't be touched by your PR.

dominikschulz · 2017-08-28T12:37:25Z

collector/cluster_health.go

@@ -220,49 +217,14 @@ func (c *ClusterHealth) Describe(ch chan<- *prometheus.Desc) {
 	ch <- c.jsonParseFailures.Desc()
 }

-func (c *ClusterHealth) fetchAndDecodeClusterHealth() (clusterHealthResponse, error) {


this method should remain in this file

dominikschulz · 2017-08-28T12:37:36Z

collector/cluster_health_test.go

-	"github.com/go-kit/kit/log"
-)
-
-func TestClusterHealth(t *testing.T) {


this method should remain in this file

dominikschulz · 2017-08-28T12:38:20Z

collector/metrics_collector.go

+	indexStatsResponse    *indexStatsResponse
+}
+
+func NewMetricsCollector(logger log.Logger, client *http.Client, url *url.URL, all bool, exportIndices bool) *MetricsCollector {


We should init all metrics collector in main, not in another wrapper method.

dominikschulz · 2017-08-28T12:38:34Z

collector/metrics_collector.go

+	c.indices.Collect(ch, clusterHealthResponse, nodeStatsResponse, indexStatsResponse)
+}
+
+func (c *ClusterHealth) fetchAndDecodeClusterHealth() (clusterHealthResponse, error) {


this method should stay in it's own file.

dominikschulz · 2017-08-28T12:38:44Z

collector/metrics_collector.go

+	return chr, nil
+}
+
+func (c *Nodes) fetchAndDecodeNodeStats() (nodeStatsResponse, error) {


this method should stay in it's own file.

dominikschulz · 2017-08-28T12:39:00Z

collector/metrics_collector_test.go

+	return
+}
+
+func TestClusterHealth(t *testing.T) {


this method should stay in it's own file.

dominikschulz · 2017-08-28T12:39:11Z

collector/metrics_collector_test.go

+	}
+}
+
+func TestNodesStats(t *testing.T) {


this method should stay in it's own file.

dominikschulz · 2017-08-28T12:39:28Z

collector/nodes.go

@@ -984,51 +525,14 @@ func (c *Nodes) Describe(ch chan<- *prometheus.Desc) {
 	ch <- c.jsonParseFailures.Desc()
 }

-func (c *Nodes) fetchAndDecodeNodeStats() (nodeStatsResponse, error) {


this method should remain in this file

dominikschulz · 2017-08-28T12:39:35Z

collector/nodes_test.go

-	"github.com/go-kit/kit/log"
-)
-
-func TestNodesStats(t *testing.T) {


this method should remain in this file

zwopir · 2017-08-28T12:01:02Z

collector/cluster_health.go

-}
-
-func (c *ClusterHealth) Collect(ch chan<- prometheus.Metric) {
+func (c *ClusterHealth) Collect(ch chan<- prometheus.Metric, clusterHealthResponse clusterHealthResponse) {


this doesn't work. Collect must fulfill the prometheus. Collector interface and thus the signature must be

Collect(ch chan<- prometheus.Metric)

zwopir · 2017-08-28T12:47:59Z

collector/metrics_collector.go

+
+	c.clusterHealth.Collect(ch, clusterHealthResponse)
+	c.nodes.Collect(ch, nodeStatsResponse)
+	c.indices.Collect(ch, clusterHealthResponse, nodeStatsResponse, indexStatsResponse)


wrapping collectors (or your custom non-interface-fulfilling Collect() ) isn't a good idea. Doing so makes the collectors run sequentially, not in parallel. Also the json retrieval works parallel, if you exclude it from the collectors.
In additionMaking collectors optional is then just a matter of ifing the prometheus.Mustregister().

Could you please refer to you initial intent of this PR: get the docs metrics of the primary shards. To archieve this I would strongly suggest to

go back to the start (sorry)

implement the indices collector (fulfilling the collector interface) with just the very few metrics included you are interested in. We would really like to keep the overall index metrics we can get via _nodes/stats in the nodes collector. Please only export the structs marshaled from the _all/_stats that are not in _nodes/stats.

add this collector optionally with prometheus.MustRegister

matsumana · 2017-08-28T17:29:02Z

@zwopir @dominikschulz
I fixed.
Would you please review again?

zwopir

Hi Matsumana,

thanks for the update. It almost looks good. Two things I would ask you to change:

exclude the flag, if indices metrics should be scraped to main.go
a copy'n'paste error in the indices Collect() func

zwopir · 2017-08-29T08:59:46Z

collector/indices.go

+			"err", err,
+		)
+		return
+	}


this clusterHealth block in the indices collector is probably a copy'n' paste error, isn't it?!

zwopir · 2017-08-29T09:03:23Z

collector/indices.go

+	client        *http.Client
+	url           *url.URL
+	all           bool
+	exportIndices bool


please move the bool controlling if indices metrics should be scraped out of the collector. There is no reason registering the metrics via Describe() and then don't collect them. See my other comment in main.go

zwopir · 2017-08-29T09:04:12Z

collector/indices.go

+func (c *Indices) fetchAndDecodeIndexStats() (indexStatsResponse, error) {
+	var isr indexStatsResponse
+
+	if c.exportIndices {


move if statement to main.go

zwopir · 2017-08-29T09:05:23Z

main.go

@@ -55,6 +56,7 @@ func main() {

 	prometheus.MustRegister(collector.NewClusterHealth(logger, httpClient, esURL))
 	prometheus.MustRegister(collector.NewNodes(logger, httpClient, esURL, *esAllNodes))
+	prometheus.MustRegister(collector.NewIndices(logger, httpClient, esURL, *esAllNodes, *esExportIndices))


please replace by

if *exExportIndices { prometheus.MustRegister(collector.NewIndices(logger, httpClient, esURL, *esAllNodes)) }

(so remove the flag from the NewIndices constructor as well)

zwopir · 2017-08-29T09:10:58Z

collector/indices.go

+				metric.Desc,
+				metric.Type,
+				metric.Value(indexStats),
+				metric.Labels(clusterHealthResponse.ClusterName, indexName)...,


copy'n'paste error

please review the labels you want to attach to the indices metrics. It seems there is not cluster name available

matsumana · 2017-08-29T09:54:13Z

@zwopir Thank you for the comments.
I fixed.
Would you please review again?

zwopir · 2017-08-29T10:04:52Z

looks good to me, thanks for your contribution!

matsumana added 2 commits August 16, 2017 21:33

add index metrics

27e56d1

re-format

0bf3dcc

dominikschulz requested review from dominikschulz and metalmatze August 16, 2017 13:38

dominikschulz added the enhancement label Aug 16, 2017

dominikschulz previously approved these changes Aug 16, 2017

View reviewed changes

matsumana mentioned this pull request Aug 16, 2017

add index metrics via alias #86

Closed

metalmatze requested changes Aug 21, 2017

View reviewed changes

zwopir requested changes Aug 21, 2017

View reviewed changes

matsumana added 2 commits August 22, 2017 01:33

moved all indices metric from collector/nodes.go to `collector/indi…

e021ec2

…ces.go`

gathered the metrics collect logic in metrics_collector

9116075

matsumana added 2 commits August 23, 2017 22:31

remove unreachable code

5f4f656

add test pattern

9bd010f

Make indices metrics optional

a3c322b

dominikschulz suggested changes Aug 28, 2017

View reviewed changes

zwopir requested changes Aug 28, 2017

View reviewed changes

matsumana added 3 commits August 29, 2017 01:25

revert

7af4dc8

revert

696489c

Make index metrics optional

9c32cc1

zwopir requested changes Aug 29, 2017

View reviewed changes

matsumana added 2 commits August 29, 2017 18:46

correct the places that were pointed out

9df62d3

fix test

6b509f3

zwopir merged commit 46d7b17 into prometheus-community:master Aug 29, 2017

This was referenced Sep 6, 2017

add documentation for the new metrics #89

Closed

How to fetch indices names #91

Closed

matsumana deleted the feature/add-index-metrics branch September 7, 2017 09:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add index metrics #85

add index metrics #85

matsumana commented Aug 16, 2017 •

edited

Loading

dominikschulz left a comment

matsumana commented Aug 21, 2017

metalmatze left a comment

zwopir left a comment •

edited

Loading

matsumana commented Aug 23, 2017 •

edited

Loading

matsumana commented Aug 23, 2017

zwopir commented Aug 25, 2017

dominikschulz commented Aug 25, 2017

matsumana commented Aug 26, 2017

dominikschulz commented Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

dominikschulz Aug 28, 2017

zwopir Aug 28, 2017

zwopir Aug 28, 2017

matsumana commented Aug 28, 2017 •

edited

Loading

zwopir left a comment

zwopir Aug 29, 2017

zwopir Aug 29, 2017

zwopir Aug 29, 2017

zwopir Aug 29, 2017

zwopir Aug 29, 2017

zwopir Aug 29, 2017

matsumana commented Aug 29, 2017

zwopir commented Aug 29, 2017

add index metrics #85

add index metrics #85

Conversation

matsumana commented Aug 16, 2017 • edited Loading

dominikschulz left a comment

Choose a reason for hiding this comment

matsumana commented Aug 21, 2017

metalmatze left a comment

Choose a reason for hiding this comment

zwopir left a comment • edited Loading

Choose a reason for hiding this comment

matsumana commented Aug 23, 2017 • edited Loading

matsumana commented Aug 23, 2017

zwopir commented Aug 25, 2017

dominikschulz commented Aug 25, 2017

matsumana commented Aug 26, 2017

dominikschulz commented Aug 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsumana commented Aug 28, 2017 • edited Loading

zwopir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsumana commented Aug 29, 2017

zwopir commented Aug 29, 2017

matsumana commented Aug 16, 2017 •

edited

Loading

zwopir left a comment •

edited

Loading

matsumana commented Aug 23, 2017 •

edited

Loading

matsumana commented Aug 28, 2017 •

edited

Loading